kernelctf: add CVE-2023-4015_cos #140
Conversation
kungfulon
force-pushed
the
exp96
branch
4 times, most recently
from
df45592 to
d09e7ce
Compare
koczkatamas
force-pushed
the
master
branch
3 times, most recently
from
933028d to
c111d81
Compare
koczkatamas
force-pushed
the
master
branch
13 times, most recently
from
76d6b57 to
bf0d68f
Compare
There was a problem hiding this comment.
Hey,
This is just a quick code quality review. We're planning to review the submissions more deeply (actually understanding what the exploit does) in two weeks.
In general, the code quality looks good, no major concerns. I've left a few smaller comments.
We also have a draft style guide now. Please take a look and let us know if it's helpful for understanding our code quality expectations: https://google.github.io/security-research/kernelctf/style_guide.
Thanks for the submission and PR!
| ); | ||
| } | ||
|
|
||
| void monke() { |
There was a problem hiding this comment.
Can you please rename this function to something else (e.g. after_privesc_as_root) and avoid the word monke in the exploit?
| } | ||
|
|
||
| static int run_callbacks(sock s, mnl_cb_t cb, void *data) { | ||
| // INFO("Start callback: rseq = %d, seq = %d", rseq, seq); |
There was a problem hiding this comment.
Not a serious concern, but why was this commented out? It is called too many times? Maybe convert to DEBUG( and give an option via defines or command line arguments to turn on/off debug messages? (Or you can just remove it).
More info in our style guide.
| struct nft_rule *fake_rule = (struct nft_rule *)data; | ||
| fake_rule->dlen = 1; // pass the check | ||
| struct nft_expr *fake_expr = (struct nft_expr *)(data + sizeof(struct nft_rule)); | ||
| fake_expr->ops = (struct nft_expr_ops *)(heap + 0x88 - 0x28); // offset 0x88 of fake rule <=> expr->ops->deactivate (offset 0x28 of expr->ops) |
There was a problem hiding this comment.
Do I understand correctly, that you selected the offset 0x88 based on where was free space in the ROP chain and 0x28 is the offset of nft_expr_ops's deactivate field?
Can you use #define NFT_EXPR_OPS_OFFS_DEACTIVATE 0x28 and use that macro?
And also move 0x88 into a variable (e.g. deactivate_offset) or define and set the JOP gadget via rop[deactivate_offset/8] = vmlinux + PUSH_RSI_JMP_QWORD_PTR_RSI_F instead of *rop++ = vmlinux + PUSH_RSI_JMP_QWORD_PTR_RSI_F;, so the connection is more clear?
More info is in our style guide.
|
|
||
| // return to userspace | ||
| *rop++ = vmlinux + KPTI_TRAMPOLINE; | ||
| *rop++ = vmlinux + PUSH_RSI_JMP_QWORD_PTR_RSI_F; // jop gadget, put here because this space is unused |
There was a problem hiding this comment.
Comment here where the RSI pointer points to (fake nft_expr?).
| return MNL_CB_OK; | ||
| } | ||
|
|
||
| static int dump_expr_leak_vmlinux(expr e, void *dat) { |
There was a problem hiding this comment.
Nothing serious, but here the unused dat argument is a bit similar to the one used below (data), maybe consider renaming dat into unused_ or something similar to make intention more clear.
No description provided.